Skip to content

feat(oiiotool): Add thumbnail get/set commands and fix TGA thumbnail I/O#5236

Open
jinhgkim wants to merge 8 commits into
AcademySoftwareFoundation:mainfrom
jinhgkim:oiiotool-thumbnail
Open

feat(oiiotool): Add thumbnail get/set commands and fix TGA thumbnail I/O#5236
jinhgkim wants to merge 8 commits into
AcademySoftwareFoundation:mainfrom
jinhgkim:oiiotool-thumbnail

Conversation

@jinhgkim

Copy link
Copy Markdown

Description

Adds oiiotool commands to extract and attach embedded thumbnails, and fixes TGA postage-stamp read/write bugs they exposed. This PR adresses #4889.

Note that thumbnail support is uneven across formats: get_thumbnail is implemented for PSD, camera RAW, and TGA, but set_thumbnail only for TGA. So you can read thumbnails from several formats, yet TGA is the only format that writes an embedded thumbnail today. e.g., input.exr ... --set-thumbnail -o out.exr silently produces an EXR with no embedded thumbnail.

  • Features:

    • oiiotool: add --get-thumbnail
    • oiiotool: add -i:get_thumbnail=1
    • oiiotool: add --set-thumbnail
  • Fixes:

    • targa thumbnail write: encode thumbnail like the main image (bottom-up, deassociated alpha, BGR(A))
    • targa thumbnail read: apply associateAlpha()

Assisted-by: Claude Code / opus-4.8

Tests

New tests in oiiotool-get-thumbnail, oiiotool-set-thumbnail, imagebuf_test

Checklist:

  • I have read the guidelines on contributions and code review procedures.
  • I have read the Policy on AI Coding Assistants
    and if I used AI coding assistants, I have an Assisted-by: TOOL / MODEL
    line in the pull request description above.
  • I have updated the documentation if my PR adds features or changes
    behavior.
  • I am sure that this PR's changes are tested in the testsuite.
  • I have run and passed the testsuite in CI before submitting the
    PR, by pushing the changes to my fork and seeing that the automated CI
    passed there. (Exceptions: If most tests pass and you can't figure out why
    the remaining ones fail, it's ok to submit the PR and ask for help. Or if
    any failures seem entirely unrelated to your change; sometimes things break
    on the GitHub runners.)
  • My code follows the prevailing code style of this project and I
    fixed any problems reported by the clang-format CI test.
  • If I added or modified a public C++ API call, I have also amended the
    corresponding Python bindings. If altering ImageBufAlgo functions, I also
    exposed the new functionality as oiiotool options.

@linux-foundation-easycla

linux-foundation-easycla Bot commented Jun 15, 2026

Copy link
Copy Markdown

CLA Signed
The committers listed above are authorized under a signed CLA.

jinhgkim added 7 commits June 14, 2026 22:42
Assisted-by: Claude Code / opus-4.8
Signed-off-by: Jinnie Kim <jinhgkim@gmail.com>
Assisted-by: Claude Code / opus-4.8
Signed-off-by: Jinnie Kim <jinhgkim@gmail.com>
Signed-off-by: Jinnie Kim <jinhgkim@gmail.com>
Assisted-by: Claude Code / opus-4.8
Signed-off-by: Jinnie Kim <jinhgkim@gmail.com>
Encode the embedded thumbnail the same way as the main image: bottom-up
scanlines, deassociated alpha, and BGR(A) channel order. The original
implementation rather dumped the raw in-memory buffer, which left the
thumbnail flipped, R/B-swapped, and premultiplied.

Also clamp an oversized thumbnail to 255 rather than 256: each postage-stamp
dimension is a single byte, so 256 wrapped to 0 and the thumbnail was
silently dropped on read.

Assisted-by: Claude Code / opus-4.8
Signed-off-by: Jinnie Kim <jinhgkim@gmail.com>
TGA stores unassociated alpha, but OIIO keeps it associated in memory.
`readimg()` converts on read; `get_thumbnail()` did not, so RGBA thumbnails came
back too bright. Apply the same `associateAlpha()` conversion to the thumbnail.

get_thumbnail() is moved below associateAlpha() so it can call it without a
forward declaration.

Assisted-by: Claude Code / opus-4.8
Signed-off-by: Jinnie Kim <jinhgkim@gmail.com>
Assisted-by: Claude Code / opus-4.8
Signed-off-by: Jinnie Kim <jinhgkim@gmail.com>
@jinhgkim jinhgkim force-pushed the oiiotool-thumbnail branch from 54e6a14 to 2b002d9 Compare June 15, 2026 05:42
Signed-off-by: Jinnie Kim <jinhgkim@gmail.com>
@jinhgkim jinhgkim force-pushed the oiiotool-thumbnail branch from 2b002d9 to c589b11 Compare June 15, 2026 05:44
@jinhgkim

jinhgkim commented Jun 15, 2026

Copy link
Copy Markdown
Author

During the CI run, a TGA thumbnail test failed because I added associateAlpha() logic in this branch. My reasoning is that if we apply associateAlpha() in TGAInput::readimg(), we should apply the same operation when reading thumbnails.

Let me know what you think.

Also, fyi, I have broken down the commits into logical steps with some details in the commit message.

@lgritz lgritz left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This more or less LGTM. I didn't click approve only because I want to give you the chance to respond to some of the comments I left, none of which are strictly required, but maybe you'll particularly want to do some of those things.

Comment thread src/doc/oiiotool.rst
Comment on lines +2365 to +2367
If 0, an empty (0x0) image is pushed in its place instead, so a batch
script can continue; guard any subsequent output (see the example
below), since writing the empty image is itself an error.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things to consider here.

  1. I'm not sure how resilient anything downstream will be to a literal 0x0 image.
  2. Does it make more sense for a "handled failure" to be either (a) just leave the original full-size image on the stack, or (b) perform the equivalent of a --resize or --resample to a reasonable thumbnail-like resolution?

Comment thread src/oiiotool/oiiotool.cpp
Comment on lines +7510 to +7515
ap.arg("--get-thumbnail")
.help("Extract an embedded thumbnail (options: fail=, index=)")
.OTACTION(action_get_thumbnail);
ap.arg("--set-thumbnail")
.help("Attach the top image as the thumbnail of the image below it")
.OTACTION(action_set_thumbnail);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I feel strongly about this, but want to throw it out there in case it resonates with you. Would you prefer "--thumbnail-get" and "--thumbnail-set" for the sake of ensuring that they are always next to each other in an alphabetical listing of commands?

We purposely picked --mulc so that it would be next to --mul in alphabetical listings for similar reason.

Comment on lines +638 to +643
bool
TGAInput::get_thumbnail(ImageBuf& thumb, int subimage)
{
if (m_ofs_thumb <= 0)
return false; // no thumbnail info

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can I ask why this function moved position in the file? That makes it hard to tell form the diffs exactly what changed. Or did it only move?

@lgritz

lgritz commented Jun 15, 2026

Copy link
Copy Markdown
Collaborator

Just another suggestion I thought of while reviewing this, so I'll mention it here. Though you need not add it, maybe it's an item for a follow-up, or just to put on the list of extensions to consider for some day (maybe only after index= works?).

Do you think it would be useful to have a :requested=WxH hint that, if more than one thumbnail were present, would pick the one closest to what you ask for? You could even imagine a few variants that extend it, like :requested=<WxH means the largest thumbnail that fits in WxH, :requested=>WxH means "the smallest thumbnail at least that size" and :requested=WxH! means "I really need that size exactly, pick the best but also resize/fit if you need to."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants